Respect MFMA arch disablement in CI - #2455
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Jenkins pipeline logic for the mfma codepath so that MFMA CI jobs only run on explicitly enabled MFMA-capable architectures (gfx908/gfx90a/gfx942), matching existing “disable arch” behavior used elsewhere in the pipeline.
Changes:
- Add a helper to compute the set of enabled MFMA architectures from
disable908/disable90a/disable942. - Build the MFMA Jenkins node label dynamically from the enabled architecture set.
- Prevent MFMA codepath execution when all supported MFMA architectures are disabled.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2455 +/- ##
===========================================
+ Coverage 82.57% 83.61% +1.04%
===========================================
Files 120 121 +1
Lines 42852 43182 +330
Branches 7110 7181 +71
===========================================
+ Hits 35381 36104 +723
+ Misses 4815 4504 -311
+ Partials 2656 2574 -82
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
justinrosner
added
the
claude-review
![rocmlir-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/7440191?s=60&v=4){kind=small}
| // Run mfma on private CI | ||
| if ((codepath == "mfma") && params.canXdlops) { | ||
| if ((codepath == "mfma") && params.canXdlops && | ||
| !getEnabledMfmaArchitectures().isEmpty()) { |
There was a problem hiding this comment.
Worth calling out a side effect of skipping the whole mfma row: preMergeCheck() (Jenkinsfile:906) and preMergeCheckPackage() (Jenkinsfile:934) are both gated on codepath == "mfma", and on private CI (canXdlops == true) there is no vanilla row to pick them up. So a run with all three MFMA archs disabled now silently loses the Jenkins-side premerge static check and the librockcompiler_deps.cmake accuracy check, with no log line saying why. Suggest either an echo here when the enabled list is empty (so the skip is visible in the build log), or moving those two checks onto a codepath-independent row so they survive an MFMA node outage.
There was a problem hiding this comment.
Verdict: COMMENT · Findings: 1 (0 Critical, 0 Major, 1 Minor)
Scope
Single-file change to mlir/utils/jenkins/Jenkinsfile. It adds getEnabledMfmaArchitectures() and uses it to (a) build the MFMA Jenkins node label from only the enabled gfx942/gfx908/gfx90a archs, (b) make shouldRunFromCodepath("mfma") and shouldRunBuildAndTest("mfma") return false when all three are disabled, and (c) gate the Code coverage matrix row on shouldRunFromCodepath(CODEPATH).
Findings
mlir/utils/jenkins/Jenkinsfile:1171(Minor) — skipping the wholemfmarow also silently skips the mfma-only premerge static checks; consider surfacing a warning or relocating those checks.
Notes
- Verified all four
getLabelFromCodepathcall sites are guarded before the newerrorat :1033 can fire — Build and Test (:1533, undershouldRunBuildAndTestat :1527), Parameter sweeps (:1689, under :1683), MIGraphX (:2211, under :2205), and Code coverage (:2342, under the newly addedwhenat :2333). This resolves the earlier Copilot review comment; theerroris now purely defensive against a future unguarded caller. - With nothing disabled the generated label is byte-identical to the old literal (
mlir && (gfx942 || gfx908 || gfx90a)), so the default private-CI path is unchanged.labelis declaredStringat :1029, so the new GString is coerced before it reacheswithHealthyNode. - The new
whenonCode coveragealso gates thegfx120xandgfx950rows, not justmfma. All three coverage codepaths requireparams.canXdlops, so on public CI the stage now yields zero rows instead of failing to find a node and being swallowed bycatchError. That reads as an improvement, but it is broader than the PR description states — worth confirming it is intended. getEnabledMfmaArchitectures()uses theparams.disableX == falseidiom, consistent with :1174/:1178. Note the failure mode is asymmetric: a null or string-typed param evaluates as "disabled", which would now silently skip all MFMA CI rather than a single arch. Consistent with existing code, so not flagged.getLabelFromChip()(:1068) keeps its per-chip literals; those rows are already gated byshouldRunFromChip, which consults the same disable flags, so no divergence was introduced.- No Lit/E2E test expectations apply here — this is CI orchestration only, with no IR, C-API, or
librockcompilersurface change, so no coordinated MIGraphX update is needed.
CI status
No checks in /tmp/pr/checks.json are in bucket: fail or cancel. Several Jenkins stages (Build and Test, MIGraphX, Code coverage, Parameter sweeps) are still pending, which is expected mid-run.
rocmlir-pr-reviewer
Bot
removed
the
claude-review
Motivation
Ensure PR and nightly MFMA CI jobs respect the architecture disable parameters. Previously, MFMA jobs could still run on any one of gfx908, gfx90a, or gfx942 even if those archs were explicitly disabled. This now matches the behavior that we can get for other archs (gfx950, Navi3x, etc.)
This is a port of https://github.com/ROCm/rocmlirTriton/pull/427 from rocmlirTriton.
Technical Details
NOTE: rocMLIR already respects these disable flags in
shouldRunFromChip()for chip specific tuning and performance matrix rows. However, build/test and other codepath-based rows still needed updating.Test Plan
Test Result
Submission Checklist